Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update CI pipeline to remove actions-rs #23

Merged
merged 1 commit into from
Aug 13, 2024
Merged

Conversation

pulsastrix
Copy link
Member

As the actions-rs GitHub actions are no longer maintained, this PR replaces those with suitable alternatives.

Strongly inspired by the CI pipeline in namib-project/libcoap-rs and the grcov README

@pulsastrix pulsastrix self-assigned this Aug 9, 2024
@coveralls
Copy link

coveralls commented Aug 10, 2024

Coverage Status

coverage: 84.95%. remained the same
when pulling 554e607 on update_ci_pipeline
into 0f6a7c2 on main.

@pulsastrix
Copy link
Member Author

I've also replaced the current coverage tool (grcov) with tarpaulin, although I am unsure if this should be kept in.

With grcov, I had some issues with excluding directories. I got an obscure missing file issue when adding --ignore /* as was previously done by the actions-rs version of grcov, but not adding it causes some non-code files to be included (no idea where the coverage values for those come from?!?), see https://coveralls.io/builds/69151273.

cargo tarpaulin however seems to have a bunch of false negatives and/or positives, see https://coveralls.io/builds/69151472/source?filename=src%2Ftoken%2Fcose%2Fmaced%2Fmod.rs#L208 for instance.

Considering that the coverage numbers seem to be vastly different when comparing the current state of main with the one of this PR, at least one of the two different coverage reporting tools seems to be very off.

Any opinions on this (pinging @falko17)?

@falko17
Copy link
Member

falko17 commented Aug 10, 2024

Considering that the coverage numbers seem to be vastly different when comparing the current state of main with the one of this PR, at least one of the two different coverage reporting tools seems to be very off.

Any opinions on this (pinging @falko17)?

Hm, we should investigate more deeply why these numbers are so different (67% vs. 85%, if I interpret this correctly). Given the extensive tests we have in place, I trust the 85% number more, but this could also be too optimistic—we need to take a deeper look into it before we decide on which tool to use.

@pulsastrix
Copy link
Member Author

As tarpaulin seems to have a number of false negatives, I have switched back to grcov and fixed the aforementioned issues.

The line coverage calculation now seems to be fine, the coverage percentage being lower is due to the branch coverage being aggregated with the line coverage, see https://coveralls.io/jobs/149319128.

Should I keep this setting enabled? It can be disabled in https://coveralls.io/github/namib-project/dcaf-rs/settings.

@pulsastrix pulsastrix marked this pull request as ready for review August 13, 2024 15:27
@falko17
Copy link
Member

falko17 commented Aug 13, 2024

Should I keep this setting enabled? It can be disabled in https://coveralls.io/github/namib-project/dcaf-rs/settings.

I don't think branch coverage is necessary/useful for this project, so I've disabled the aggregation for now so the number is easier to interpret (and higher :D).

falko17
falko17 previously approved these changes Aug 13, 2024
Copy link
Member

@falko17 falko17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one minor comment, so I'll approve already, thanks for the changes.

.github/workflows/check.yml Outdated Show resolved Hide resolved
@pulsastrix pulsastrix merged commit d087ba7 into main Aug 13, 2024
10 checks passed
@pulsastrix pulsastrix deleted the update_ci_pipeline branch August 13, 2024 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants